Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean up hash-to-field #678

Merged
merged 26 commits into from
Sep 29, 2023
Merged

Clean up hash-to-field #678

merged 26 commits into from
Sep 29, 2023

Conversation

burdges
Copy link
Contributor

@burdges burdges commented Sep 8, 2023

Description

Fixes poor usage of Digest, unnecessary allocations, poor style, etc. Nothing complex here.

Please merge #677 first.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@burdges burdges requested review from a team as code owners September 8, 2023 01:46
@burdges burdges requested review from Pratyush, mmagician and weikengchen and removed request for a team September 8, 2023 01:46
mmagician
mmagician reviewed Sep 8, 2023
View reviewed changes
ff/src/fields/field_hashers/mod.rs Show resolved Hide resolved
ff/src/fields/field_hashers/mod.rs Show resolved Hide resolved
ff/src/fields/mod.rs Outdated Show resolved Hide resolved
ff/src/fields/field_hashers/expander/mod.rs Outdated Show resolved Hide resolved
ff/src/fields/field_hashers/expander/mod.rs Outdated
@@ -2,98 +2,116 @@
// With some optimisations

use ark_std::vec::Vec;
use digest::{DynDigest, ExtendableOutput, Update};

use digest::{FixedOutputReset, ExtendableOutput, Update};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So IIUC Digest is a supertrait of FixedOutputReset and we just don't need all of supertrait functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot use Digest + ExtendableOutput which makes Digest useless in practice.

@burdges burdges force-pushed the clean_h2_ff branch 2 times, most recently from db9947a to 6ec3708 Compare September 8, 2023 21:35
@burdges
Copy link
Contributor Author

burdges commented Sep 8, 2023

Just fyi, I've also pulled out a branch which contains the larger subsequent changes to hash-to-field: https://github.com/burdges/algebra/commits/xof_h2f It looks easier if it's merged with some curve changes though.

Pratyush
Pratyush approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@Pratyush Pratyush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once tests are fixed!

@burdges
Copy link
Contributor Author

burdges commented Sep 19, 2023

It's been a while so I've forgotten how to trigger the expander/tests.rs to be built locally. lol

I figured this out one before. I'd though it was cargo test form the repo root, but that doesn't seem to do it right now. I should look at the CI maybe.

@burdges
Copy link
Contributor Author

burdges commented Sep 19, 2023

CI is happy now

@Pratyush
Copy link
Member

There's a little conflict; you'll have to move the digest dependency to the root Cargo.toml

@burdges
Copy link
Contributor Author

burdges commented Sep 27, 2023

Alright fixed I think, thanks :)

@Pratyush Pratyush merged commit 4152c41 into arkworks-rs:master Sep 29, 2023
15 checks passed
aleasims pushed a commit to NilFoundation/arkworks-algebra that referenced this pull request Oct 18, 2023
@burdges @Pratyush @aleasims
Clean up hash-to-field (arkworks-rs#678)
9469e1b 
Co-authored-by: Pratyush Mishra <[email protected]>
Co-authored-by: ¨Jeff <¨[email protected]¨>
aleasims added a commit to NilFoundation/arkworks-algebra that referenced this pull request Oct 18, 2023
@aleasims
Revert "Clean up hash-to-field (arkworks-rs#678)"
b1499a7 
This reverts commit 9469e1b.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmagician mmagician mmagician left review comments

@Pratyush Pratyush Pratyush approved these changes

@weikengchen weikengchen Awaiting requested review from weikengchen weikengchen is a code owner automatically assigned from arkworks-rs/maintainers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@burdges @Pratyush @mmagician